Skip to content

SAMZA-2294: LocalApplicationRunner support for missing coordinator stream#1130

Merged
xinyuiscool merged 1 commit into
apache:masterfrom
dnishimura:samza-2294-lar-no-coordinator-stream
Aug 7, 2019
Merged

SAMZA-2294: LocalApplicationRunner support for missing coordinator stream#1130
xinyuiscool merged 1 commit into
apache:masterfrom
dnishimura:samza-2294-lar-no-coordinator-stream

Conversation

@dnishimura
Copy link
Copy Markdown
Contributor

Certain standalone configurations do not define a job.coordinator.system because a coordinator stream is not necessary. For example, in PassthroughJobCoordinator configured jobs or integration test jobs might not have this defined. This PR allows support for such scenarios.

Added additional unit tests and verified by @xinyuiscool with his testing.
@xinyuiscool - Please take a look when you get a chance.

Copy link
Copy Markdown
Contributor

@xinyuiscool xinyuiscool left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the quick fix!

Copy link
Copy Markdown
Contributor

@shanthoosh shanthoosh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@shanthoosh
Copy link
Copy Markdown
Contributor

@dnishimura
Just curious. Please correct me if I'm wrong.

Do we know why TestRunner integration tests did not fail before this fix, since TestRunner uses both LocalApplicationRunner and PassThroughCoordinator?

@dnishimura
Copy link
Copy Markdown
Contributor Author

dnishimura commented Aug 7, 2019

@dnishimura
Just curious. Please correct me if I'm wrong.

Do we know why TestRunner integration tests did not fail before this fix, since TestRunner uses both LocalApplicationRunner and PassThroughCoordinator?

@shanthoosh
I have to verify, but I think it had job.coordinator.system defined.

@xinyuiscool xinyuiscool merged commit affded2 into apache:master Aug 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants